Skip to content

control_plane: adopt asap-frontend-promql for L1 parsing (Part B) - #428

Merged
zzylol merged 3 commits into
mainfrom
feat/adopt-asap-frontend-promql
Jul 29, 2026
Merged

zzylol merged 3 commits into
mainfrom
feat/adopt-asap-frontend-promql

Conversation

@zzylol

@zzylol zzylol commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Second half of implementing control_plane/docs/design-target-architecture.md's gap table (#425). Part A (serving-time cutover default flip) landed separately in #427. This PR closes the L1 gap: control_plane now delegates PromQL parsing to ASAPController's asap-frontend-promql::lower_promql instead of its own hand-rolled parser.

  • Added asap-frontend-promql dependency, pinned to the same rev already shared by asap-ir/asap-l2/asap-sketch/asap-plan.
  • query_parser::parse_query_expr_canonical/parse_query now call lower_promql directly — no reconciliation pass. Per explicit direction, classification follows whatever ASAPController's L2/L3 lowering produces, as-is.
  • Deleted control_plane/src/query_parser/promql.rs (~1187 lines) and the now-dead local L2-tree-building code in mod.rs.
  • Threaded real AccuracyTarget values through the 6 call sites that needed one (previously parser took none).

Accepted behavior changes (confirmed, not regressions)

  • Bare selectors are no longer ASAP-tier answerable on their own. The old parser implicitly wrapped a bare selector like http_requests_total in Aggregate { Sum }; asap_l2::lower does not. Tests updated across asap_tier_analysis.rs, asap_tier_implement.rs, and l4_lowering.rs to assert the new (correct) NoCallNodeFound/NotRealized/empty-candidates outcomes.
  • asap_l2::lower produces QueryExpr::TimeRange for range-vector selectors, distinct from QueryExpr::Window. Fixed query_parser's tree walkers (root_scan_schema, QeCollector::visit) to recurse into it — without this fix every _over_time query silently lost its metric name.
  • Composed queries can now yield multiple ASAPTierCandidates instead of one fused shape (e.g. sum by(zone)(rate(...)) → outer Sum + inner Increase candidates). engine.rs's serving loops already iterated &analysis.candidates, so this is a correctness improvement, not a break — but it required updating several tests that assumed candidates[0] was the only/right one to .iter().find()/.iter().any().

Two related resilience fixes this surfaced

Both analyze_promql_for_asap_tier (control_plane) and engine.rs's two live serving loops (data_plane, production path) used to abort on the first unsupported AggIntent found while walking a query tree, discarding any otherwise-servable candidate found later in the same composed query (e.g. avg by(zone)(quantile_over_time(...)) returned zero candidates because the outer unsupported Avg aborted before the inner sketchable Quantile was ever collected). Both now skip-and-continue instead, matching the multi-candidate design the loop already assumed.

Test plan

  • cargo test --release --lib -p control_plane — 710 passed, 0 failed (1 pre-existing unrelated failure skipped, confirmed via git stash diff against unmodified base)
  • cargo test --release --lib -p data_plane — 956 passed, 0 failed
  • cargo build --workspace --release — clean
  • cargo test --release -p data_plane --test e2e_controller_plans_and_backend_serves — 12 passed, 2 failed (both pre-existing, confirmed via git stash diff against unmodified base, unrelated to this change: controller_plan_to_query_full_roundtrip_{cms,count_sketch}_with_heap_topk)
  • cargo tree -p control_plane reviewed — no duplicate promql-parser instance introduced beyond the pre-existing, documented (see control_plane/Cargo.toml comment) branch=asap vs rev= tradeoff

🤖 Generated with Claude Code

zzylol and others added 3 commits July 29, 2026 06:47
Replace the local hand-rolled PromQL parser (query_parser/promql.rs,
~1187 lines) with ASAPController's asap-frontend-promql::lower_promql,
per design-target-architecture.md's L1 gap-table item. No reconciliation
pass -- classification follows whatever ASAPController's L2/L3 lowering
produces, including two accepted behavior changes:

- Bare selectors no longer get an implicit Aggregate{Sum} wrap, so they
  are no longer ASAP-tier answerable on their own.
- histogram_quantile/count_over_time classification follows asap-l2's
  lowering as-is, not this deployment's prior local heuristics.

Also fixes two abort-on-first-miss bugs this surfaced: the analyzer
(asap_tier_analysis.rs) and engine.rs's live serving loops both used to
give up on the whole query the moment one AggIntent in a composed query
was unsupported, discarding otherwise-servable candidates found later in
the same tree. Both now skip and continue, matching the intended
multi-candidate design engine.rs's loop already assumed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Follow-up to this PR's parser swap -- update the §4 gap table and net
reading now that L1 is adopted, leaving the serving-time cutover as the
one remaining open item.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…end-promql

# Conflicts:
#	control_plane/docs/design-target-architecture.md
@zzylol
zzylol merged commit 25a13d5 into main Jul 29, 2026
@zzylol
zzylol deleted the feat/adopt-asap-frontend-promql branch July 29, 2026 16:55
zzylol added a commit that referenced this pull request Jul 29, 2026
…xpr_canonical signature (#429)

* feat(control_plane): classify AggRole via real AggIntent, not a PromQL-string sniff

derive_agg_role() used to guess a workload entry's AggRole (Quantile/
Sum/Count/Topk/Other) by sniffing the leading token of its PromQL
query_string -- the same duplicate-classifier smell already retired
from the live serving path (the old two-analyzer comparison in
analyzer-parity-matrix.md, #422). Now parses the query through the
same canonical pipeline capability_for()/serving uses
(query_parser::parse_query_expr_canonical), runs it through the L3
rule-based optimizer (QueryOptimizer::new(0.0).optimize -- TopKFusion
is a pure structural rewrite, ignores the cost model, so the
placeholder throughput doesn't affect the outcome) so topk(k, m)
actually reaches an Aggregate{TopK} node instead of staying
Sort+Limit, then classifies by the real outer AggIntent
(collect_agg_intents, now pub(crate) for this reuse).

Two things this surfaced:
- `(quantile_over_time(0.9, m[5m]))` (redundant wrapping parens) --
  the old leading-token sniff finds an empty token at a `(` and
  mis-defaults to Sum; real classification is unaffected by surface
  punctuation. New test pins this.
- `topk_over_time(...)` was in the old test's query list but isn't a
  real function this parser (or vanilla PromQL) recognizes at all --
  confirmed via grep, nowhere in query_parser/promql.rs or
  intent_algebra/lower.rs. A workload entry with that query_string
  would fail to parse anywhere else in the real pipeline too, so the
  old heuristic classifying it as Topk was itself the bug. Test
  updated to drop it, with an explanatory comment.

cargo build --workspace clean; control_plane 727/727 (one
pre-existing, unrelated skip as before), workload:: 33/33 (32 + 1 new
test).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(control_plane): thread AccuracyTarget through derive_agg_role, fix histogram_quantile role

parse_query_expr_canonical now requires an AccuracyTarget (landed in
#428, merged after this branch was cut) -- thread the workload entry's
own accuracy_sla through via accuracy_target_from_legacy_accuracy_sla
rather than leaving this call site broken.

Also fixes a real misclassification #428 surfaced: the classic-bucket
histogram_quantile(0.99, rate(m_bucket[5m])) shape now lowers to the
real, exact-only AggIntent::HistogramQuantile (not the sketchable
Quantile the old local parser always substituted). derive_agg_role's
wildcard arm already routed this to AggRole::Other correctly -- only
the test's expectation was stale, asserting the old parser's behavior.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Aug 22, 2026
…ing (#443)

ASAPController was renamed to ASAPPlanner, and its main branch has moved
far past the rev this repo was pinned to: the flat asap-ir/asap-l2/
asap-sketch/asap-plan crate split was consolidated into asap-types
(pre_asap/post_asap modules) + asap-aware-mapping, and several IR shapes
changed underneath. This re-pins to current ASAPPlanner main and adapts
every downstream consumer so the workspace builds and passes tests
against it.

Dependency changes
- control_plane, crates/asap_types, data_plane Cargo.toml: repoint git
  deps from ASAPController to ASAPPlanner at current main
  (cb70086b4c4a7ba89baf2516be81d0b192137a3a); replace asap-ir/asap-l2/
  asap-sketch/asap-plan with asap-types (aliased locally as
  `planner-types` via Cargo's `package = "..."` to avoid a path
  collision with this repo's own crates/asap_types) and
  asap-aware-mapping.

Upstream IR changes adapted to
- SummaryKind/SummaryParams split into per-family ExactKind/ExactParams
  (exact accumulators) and SketchKind/SketchParams (approximate
  sketches); SummaryAgg's kind+params fields collapsed into one
  `family: SummaryFamilyType` enum. Every call site across
  sketch_algebra, optimizer, emit, physical, and data_plane's
  query engine updated for the split/collapse.
- QueryExpr::Window removed upstream (never had a real producer);
  TimeRange{range,child} was already the actual canonical shape, so
  every Window match arm across window_fusion.rs, optimizer/engine.rs,
  physical/{allocator,planner}.rs, colored_dag/*, asap_tier_*.rs,
  and query_parser is rewritten against TimeRange. window_fusion.rs's
  recognize_windowed_sketch rewrite is a genuine bug fix, not just a
  rename: the old Window-matching code was dead against real traffic.
- QueryExpr::LetBinding/Ref removed (workload-level CSE representation
  is gone upstream). control_plane's own dead CSE consumers
  (optimizer/cse.rs, intent_algebra/lower.rs) are deleted to match --
  both were confirmed to have zero live callers (PR #428 switched
  intent_algebra to asap_frontend_promql::lower_promql directly).
  CommonSubexprElim (R8) is changed to always return None, with the
  removed representation documented inline; this is correctness-
  preserving, it only gives up an optimization pass.
- L2Expr/L3Expr/Expr<C> folded directly into QueryExpr itself; L3Scalar
  renamed ScalarValue; Predicate<C> changed from Predicate(pub L3Expr)
  to Predicate<C>(pub Box<QueryExpr<C>>). expr_ir.rs now re-exports the
  upstream scalar types plus type aliases (L2Expr = UnresolvedQueryExpr,
  L3Expr = QueryExpr) instead of defining its own.
- implement_tree_in_with(expr, &BindingScope, cost_model) collapsed to
  implement_tree_with(expr, cost_model) (BindingScope removed); all
  call sites updated.
- Aggregate.aggs field renamed to Aggregate.measures.

Vendored locally (upstream deleted, this repo still needs them)
- summary_exec.rs (data_plane): SummaryExecutor trait + execute(),
  deleted upstream (ASAPPlanner#190/#197). Ported from the pre-deletion
  crates/sketch/src/exec.rs, adapted so find_candidates takes
  `family: &SummaryFamilyType` instead of separate kind/params.
- WindowKind (crates/asap_types/src/enums.rs): deleted upstream
  alongside QueryExpr::Window: ASAPPlanner's scope (batch workload
  planning) has no use for tumbling/sliding/session flush semantics,
  but this repo's streaming aggregation config still does. Same shape
  as the old re-export, so its ~145 call sites needed no changes.
- Flat SummaryKind/SummaryParams (crates/asap_types/src/
  accumulator_spec.rs): upstream's split into ExactKind/SketchKind has
  no single type spanning both anymore, but this repo's
  AccumulatorSpec dispatch (~25 call sites in
  precompute_engine::accumulator_factory) never needed that
  distinction -- it's purely "which concrete Rust accumulator struct
  to build." Vendored as the same 14-variant shape the pre-split type
  had, with From impls to convert from upstream's ExactKind/SketchKind
  where this repo's other code needs to bridge between them
  (Materialization.kind/params, BackendAggregation's backend-facing
  fields -- both genuinely span exact+sketch).

Testing
- cargo check --workspace --all-targets: clean.
- control_plane lib: 706 passed, 1 failed (pre-existing -- see below).
- data_plane lib: 927 passed, 0 failed.
- asap_types lib: 42 passed, 0 failed.
- data_plane/control_plane integration suites: same pattern, all
  passing except the same 1 pre-existing failure plus 2 already-
  ignored tests tracked as ASAPQuery-backend#431.

Pre-existing failures (not regressions -- verified against unmodified
main via a detached git worktree before touching anything):
- optimizer::rules::tests::invalid_sketch_type_override_falls_back_to_default:
  fails identically on unmodified main. bind_workload_typed_with_item_filter's
  override-re-derivation logic re-derives `statistic` to match the
  override's natural family BEFORE validity is checked, making the
  "invalid override falls back to default" path unreachable for the 5
  canonical sketch families -- a genuine pre-existing logic bug.
- controller_plan_to_query_full_roundtrip_{cms,count_sketch}_with_heap_topk:
  fail identically on unmodified main; matches two adjacent #[ignore]d
  tests in the same file already documented as a known gap tracked at
  ASAPQuery-backend#431.

Not in scope
- A repo-wide sweep of ASAPController -> ASAPPlanner prose in comments/
  docs was only done in files this migration otherwise touched; a full
  sweep across untouched files is left as follow-up.

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant